Add resources azurerm_virtual_network_gateway and azurerm_virtual_network_gateway_connection #133
Conversation
…work_gateway_connection (migrated from original pull request hashicorp#13886 in hashicorp/terraform)
…etwork_gateway_connection to latest azure sdk
…m_virtual_network_gateway_connection
|
@tombuildsstuff Please consider that I commented your review comments in the original pull request #13886 and not in this one. Most comments have been addressed, however the question how to properly handle the subnet deletion issue remains open. |
|
@dominik-lekse : Just wanted to check any ETA when will this be pushed to the master branch. We have a requirement and want to use this new resource feature. Is it OK now to push the current changes to the master and improve upon the subnet deletion issue later as a new PR ? |
|
Hey @veeresh1982
Unfortunately we're in a holding pattern with this PR until the underlying Azure API is fixed - as the API's returning that the Virtual Network Gateway's been deleted when it's not. There's an Github issue tracking the bug here - and I've commented asking for an update / rough timeline for this being fixed. Whilst we could potentially merge this in now, it'd give end users random failures when applying their Terraform configs - which isn't an optimal user experience. In the interim - you should be able to provision a Virtual Network Gateway using the Thanks! |
|
Thanks for the status update on this. Besides the subnet deletion issue, did you have a chance to take a look at the changes which have been applied in accordance to your last review? |
There was a problem hiding this comment.
Hey @dominik-lekse
Thanks for porting this PR over to the new repo - apologies for the delay in reviewing this :)
I've taken another look through this PR and it's looking good - most of the points I've raised are about consistency with other resources. Unfortunately I've not heard back about the Subnet deletion issue yet - as such I'm going to reach out via some backchannels and see if I can at least get a rough estimate of when this'll be fixed, so we know how to proceed :)
Thanks!
| resourceName := "azurerm_virtual_network_gateway_connection.test" | ||
|
|
||
| ri := acctest.RandInt() | ||
| config := fmt.Sprintf(testAccAzureRMVirtualNetworkGatewayConnection_sitetosite, ri) |
There was a problem hiding this comment.
minor could we switch this over to a function returning a formatted string to match the newer resources?
| if resp.StatusCode == http.StatusNotFound { | ||
| return nil, false, nil | ||
| } | ||
| return nil, false, fmt.Errorf("Error making Read request on Azure LocalNetworkGateway %s: %s", name, err) |
There was a problem hiding this comment.
it'd be good to return the full error by making the second argument %+v
azurerm/resource_arm_subnet.go
Outdated
| // Retry deletion of gateway subnet if provisioning state is failed | ||
| if *resp.SubnetPropertiesFormat.ProvisioningState == "Failed" { | ||
| log.Printf("[DEBUG] Retry deleting Gateway Subnet %s/%s after failed provisioning state.", vnetName, name) | ||
| subnetClient.Delete(resGroup, vnetName, name, make(chan struct{})) |
There was a problem hiding this comment.
do we need to check the result of this call? in addition - do we need to implement some kind of "max attempts" here?
There was a problem hiding this comment.
This workaround should not be relevant anymore.
azurerm/resource_arm_subnet.go
Outdated
| Timeout: 15 * time.Minute, | ||
| } | ||
| if _, err := stateConf.WaitForState(); err != nil { | ||
| return fmt.Errorf("Error waiting for Gateway Subnet %s/%s to be removed: %s", vnetName, name, err) |
There was a problem hiding this comment.
can this last argument become %+v?
|
|
||
| "default_local_network_gateway_id": { | ||
| Type: schema.TypeString, | ||
| Optional: true, |
There was a problem hiding this comment.
should this also be computed?
|
|
||
| * `express_route_circuit_id` - (Optional) The full Azure resource ID of the | ||
| Express Route Circuit when creating an ExpressRoute connection. The | ||
| Express Route Circuit can be in the same or in a different subscription. |
There was a problem hiding this comment.
We'd normally phrase this as "The ID of the Express Route Circuit"
| * `local_network_gateway_id` - (Optional) The full Azure resource ID of the | ||
| local network gateway when creating Site-to-Site connection. | ||
|
|
||
| * `routing_weight` - (Optional) The routing weight. The default value is 10. |
There was a problem hiding this comment.
Can we quote 10 here as it's the value?
| The peer virtual network gateway can be in the same or in a different subscription. | ||
|
|
||
| * `local_network_gateway_id` - (Optional) The full Azure resource ID of the | ||
| local network gateway when creating Site-to-Site connection. |
There was a problem hiding this comment.
We'd normally phrase this as "The Local Network Gateway ID"
|
|
||
| * `peer_virtual_network_gateway_id` - (Optional) The full Azure resource ID | ||
| of the peer virtual network gateway when creating a VNet-to-VNet connection. | ||
| The peer virtual network gateway can be in the same or in a different subscription. |
There was a problem hiding this comment.
We'd normally phase this as "The ID of the Peer Virtual Network Gateway"
| * `shared_key` - (Optional) The shared IPSec key. | ||
|
|
||
| * `enable_bgp` - (Optional) If true, BGP (Border Gateway Protocol) is enabled | ||
| for this connection. By default, BGP is disabled. |
There was a problem hiding this comment.
Can we quote the values and change the last sentence to "Defaults to false (disabled)" to match the other resources?
|
I'm taking a look at this PR right now and trying to build the provider, how would I pull it down and run it locally? |
|
Hey @KandrewC
There's instructions on the README for how to build and run this - however bear in mind PR's aren't fully tested and may not necessarily be 100% bug free. As such in the interim we'd recommend using the Thanks! |
# Conflicts: # azurerm/provider.go # website/azurerm.erb
…_network_gateway_connection based on latest review
…d TestAccAzureRMVirtualNetworkGatewayConnection_importSiteToSite
|
@tombuildsstuff Thanks for the review, in the the latest changes I have addressed most of your comments. Regarding the subnet deletion issue, I have another idea for a workaround which I wanted to discuss before implementing: In |
|
Looks great. I'm looking forward to it being merged 👍 |
|
@tombuildsstuff As @MohitGargVpn announced that the fix for the subnet deletion issue will be released in the Azure Api soon, I will remove the workaround from the subnet resource in this pull request and we wait for the acceptance tests to turn green. |
|
Is there a beta where I can test this? |
|
@mynkow You could checkout the pull request and build it yourself. Not trivial by any means, but not terribly difficult, either. There are build instructions in the project readme, and and little bit about pulling in custom your own plugin binaries in the Terraform docs under Running Terraform in Automation. Should really be it's own section and cross-linked from plugin development, but if I have time I'll raise an issue about that. |
|
Hi All, Thanks and your work on this is very much appreciated, |
|
@fstuck37 we're still waiting on Microsoft to fix the API which is scheduled for the end of next week, we'll take another look after that :) |
# Conflicts: # azurerm/provider.go # azurerm/resource_arm_subnet.go # azurerm/validators.go
…urce virtual_network_gateway_connection
…are not in schema anymore
|
@dominik-lekse not yet - I've been working through the |
# Conflicts: # azurerm/provider.go # azurerm/resource_arm_local_network_gateway.go
…y_connection to the v12 networking sdk
…irtual_network_gateway_connection to the v12 networking sdk
|
@tombuildsstuff With the recent migration to the v12 networking sdk in #738, I also migrated the resources of this pull request. Finally the good news is, that the eventual consistency bug in the Azure API is fixed and that the v12 SDK handles the return code in the correct way. Up to now, I only run the test Running the other tests is pending since each test takes ~45 minutes for the virtual network gateway to create. Could you take over and run these tests on the HashiCorp CI? |
|
@dominik-lekse awesome - that's great news! Sure - I'll kick off the test suite now :) Thanks! |
|
@dominik-lekse all the tests are passing 🎉 I'll take another look through this shortly, but that's really exciting :)
|
|
Sounds great, of course I expected this ;) Since the original development of these provider resources, Azure has added support for custom IPsec/IKE policies (see https://docs.microsoft.com/en-us/azure/vpn-gateway/vpn-gateway-ipsecikepolicy-rm-powershell). I suggest to add them in a separate pull request for the sake of closing this one. Adding the custom policies to the provider would be great since this is up to now neither supported by the Portal nor by the official Azure CLI. |
# Conflicts: # azurerm/provider.go
|
Resolved the conflicts with the previous commit. Ready to merge in my view. |
- Updated the examples for consistency - Fixing a highlighting issue in the sidebar - Adding a note that VNG's take a long time to provision
There was a problem hiding this comment.
hey @dominik-lekse
Thanks for pushing those updates (and resolving the conflicts) - I've spent some time testing this and have pushed some commits which resolve the issues I've pointed out below and add some extra test cases (I hope you don't mind!) - but this now LGTM.
Since the original development of these provider resources, Azure has added support for custom IPsec/IKE policies (see https://docs.microsoft.com/en-us/azure/vpn-gateway/vpn-gateway-ipsecikepolicy-rm-powershell). I suggest to add them in a separate pull request for the sake of closing this one.
Yeah definitely - I'll open another issue to keep track of that
Thanks for all the work on the Virtual Network Gateway (+ Connection) Resources, it's great to see this ship :)
Thanks!
| import ( | ||
| "github.com/hashicorp/terraform/helper/acctest" | ||
| "github.com/hashicorp/terraform/helper/resource" | ||
| "testing" |
There was a problem hiding this comment.
minor could we sort the imports here?
| import ( | ||
| "github.com/hashicorp/terraform/helper/acctest" | ||
| "github.com/hashicorp/terraform/helper/resource" | ||
| "testing" |
There was a problem hiding this comment.
minor could we sort the imports here?
| }, | ||
|
|
||
| "vpn_client_configuration": { | ||
| Type: schema.TypeSet, |
There was a problem hiding this comment.
in testing this - it appears this can become a List - I'll push a commit for this
| }, | ||
|
|
||
| "bgp_settings": { | ||
| Type: schema.TypeSet, |
There was a problem hiding this comment.
from testing it appears we can make this a List - I'll push a commit for this
|
Virtual Network Gateway & Virtual Network Gateway Connection tests pass:
|
|
Thanks for the improvements and merging the resources. |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks! |
ghost
locked and limited conversation to collaborators
This pull request adds the resources
azurerm_virtual_network_gatewayandazurerm_virtual_network_gateway_connectionwhich can be used to manage Azure VPN Gateways and Connections.It has been migrated from the pull request hashicorp/terraform#13886 in the original terraform repository after provider separation.